-
Notifications
You must be signed in to change notification settings - Fork 4
86b82fa34 - feat: add url fragment linking to tabs on edit sponsor page #751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe edit sponsor page switches tab state to URL hash fragments. Added mapping and helpers to convert between tab indices and hash fragments, initialize selected tab from the hash, and a useEffect to keep state synchronized with window.location.hash; tab changes update the URL hash. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant PageComponent
participant TabState
Note over Browser,PageComponent: Initial Load
Browser->>PageComponent: load with URL hash
PageComponent->>Browser: read window.location.hash
PageComponent->>PageComponent: getTabFromUrlFragment()
PageComponent->>TabState: initialize selectedTab
TabState-->>PageComponent: state set
Note over Browser,PageComponent: User selects tab
Browser->>PageComponent: user clicks tab
PageComponent->>PageComponent: onTabChange -> getFragmentFromValue()
PageComponent->>Browser: set window.location.hash
PageComponent->>TabState: update selectedTab
Note over Browser,PageComponent: External hash change
Browser->>PageComponent: window.location.hash changes
PageComponent->>PageComponent: useEffect triggers
PageComponent->>TabState: resync selectedTab
TabState-->>PageComponent: state updated
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/pages/sponsors/edit-sponsor-page.js:
- Around line 116-118: The useEffect using window.location.hash in the
dependency array won't re-run on hash changes; replace it with a hashchange
event listener: in the effect that currently calls
setSelectedTab(getTabFromUrlFragment()), register
window.addEventListener('hashchange', handler) where handler calls
setSelectedTab(getTabFromUrlFragment()), call the handler once on mount to
initialize, and remove the listener in the cleanup; remove window.location.hash
from the dependency array (use []).
🧹 Nitpick comments (1)
src/pages/sponsors/edit-sponsor-page.js (1)
46-64: Helper functions look good, consider consolidating the tab definition.The mapping and helper functions are well-structured. The fallback to index 0 for unrecognized fragments correctly implements the PR requirement.
Consider deriving
tabsToFragmentMapfrom thetabsarray (or vice versa) to avoid maintaining two separate lists that must stay in sync. For now, this is acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/sponsors/edit-sponsor-page.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (2)
src/pages/sponsors/edit-sponsor-page.js (2)
120-123: LGTM!Setting
window.location.hashcorrectly updates the URL and adds a browser history entry, enabling back/forward navigation between tabs.
155-155: Formatting change acknowledged.Minor whitespace adjustment in the sx prop—no functional impact.
smarcet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niko-exo please review
| const result = tabsToFragmentMap.indexOf( | ||
| window.location.hash.replace("#", "") | ||
| ); | ||
| return result > -1 ? result : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the initial load
general tab should be selected hence #general should be show by default
if (result > -1) return result;
if (window.location.hash) window.location.hash = "general";
return 0
```;
| const [selectedTab, setSelectedTab] = useState(0); | ||
| const [selectedTab, setSelectedTab] = useState(getTabFromUrlFragment()); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niko-exo
rite now the tab selection only updates from the URL hash on initial page load. The effect useEffect(..., [window.location.hash]) won’t reliably fire on hash updates because changing window.location.hash doesn’t trigger a React re-render.
To support SPA-style hash navigation (manual hash edits, back/forward), please replace that effect with a hashchange listener
22f0d4b to
9c4adf3
Compare
ref: https://app.clickup.com/t/86b82fa34
86b82fa34 - feat: add url fragment linking to tabs on edit sponsor page
Changelog
Links
86b82fa34 - Add TAB deep-linking using URL fragment
Evidence
2026-01-12_09-56-27.mp4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.